feat(sea): Thrift-parity — params, intervals, getInfo, SQL-error class, input validation + queryTags#403
Open
msrathore-db wants to merge 5 commits into
Conversation
Three driver-side parity fixes validated via the node comparator (thrift-vs-SEA on a live warehouse); params/queryTimeout are intentionally out of scope (positional params land via the kernel TypedValue codec). - intervals: default `intervalsAsString: true` on the SEA connection (SeaAuth) so INTERVAL columns surface as strings like the Thrift driver, and map a physical-Utf8 column carrying INTERVAL type-name metadata to STRING in SeaArrowIpc (the kernel keeps the INTERVAL metadata even when rendering as string). Result: interval columns match Thrift in both type code (7) and value. Requires the kernel napi `intervalsAsString` knob. - getInfo: synthesize the three TGetInfoTypes the Thrift server answers — CLI_SERVER_NAME / CLI_DBMS_NAME = "Spark SQL", CLI_DBMS_VER = "3.1.1" — client-side (JDBC DatabaseMetaData style; SEA/kernel has no getInfo RPC), and reject the rest like the server does. New SeaServerInfo.ts. - error class: map kernel `SqlError` (server execution failures — PERMISSION_DENIED, SCHEMA_ALREADY_EXISTS, bad SQL) to OperationStateError(ERROR) instead of the base HiveDriverError, matching the Thrift backend's operation-status error class. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Removes the M0 param deferral: SeaSessionBackend.executeStatement now forwards positional `?` bindings to the kernel via the napi `ExecuteOptions.positionalParams` surface (kernel #84 / TypedValueInput `{ sqlType, value? }`). New SeaPositionalParams.ts reduces each `DBSQLParameter | value` to that shape — reusing DBSQLParameter's type-inference + stringification — adapting DECIMAL to the parenthesised `DECIMAL(p,s)` form the kernel codec requires and mapping NULL to a value-less VOID input. Also wires `queryTimeout` → `queryTimeoutSecs`. Named params stay rejected (positional-only on the napi surface today). Validated live: all PREPARED_STATEMENT_TYPES cases bind correctly (INT/BIGINT/DECIMAL/STRING/BOOLEAN/DATE/TIMESTAMP/NULL + interval CAST). 201 sea unit tests pass. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
changed the title
Completes SEA param parity: SeaSessionBackend now forwards `namedParameters`
to the kernel via `ExecuteOptions.namedParams` (kernel `param_named` /
`NamedTypedValueInput {name, sqlType, value?}`), alongside positional. New
`buildSeaNamedParams` reuses the same `toTypedValueInput` mapping (DECIMAL →
DECIMAL(p,s), NULL → VOID). Positional and named are mutually exclusive
(ParameterError, matching the Thrift backend).
Requires kernel napi `namedParams` (extends the positional codec, #84 line).
Validated live: named INT/STRING/DECIMAL/multi-param bind; both-forms rejected.
205 sea unit tests pass.
Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
changed the title
…me param guards Three SEA-adapter input-validation fixes from the jira-candidate triage, all in the node layer (no kernel change), all parity-preserving vs the Thrift backend: - Metadata empty-string args (jira #2026-05-22-sea-rejects-empty-string-args): the kernel's Identifier/LikePattern reject "" with InvalidArgument while Thrift treats "" as "unspecified" (match-all/default). getSchemas/getTables/ getColumns/getFunctions now coerce "" -> undefined before the napi call (emptyToUndefined), restoring Thrift parity. Live: getSchemas(catalog="") now returns rows instead of throwing ParameterError. - Array/object param values (jira #2026-05-25-thrift-array-ordinal-hangs): an array bound as a parameter stringified to "1,2,3" and the operation never returned to FINISHED (DoS). Reject array/object values at bind time (assertBindableValue) on both positional and named paths; Date/Int64/ scalars/DBSQLParameter are allowed. - Ordinal arity (jira #2026-05-25-thrift-ordinal-excess-silent): excess ordinal params were silently dropped server-side (data-correctness footgun). Validate positionalParams.length === '?' marker count, with a quote/comment- aware scanner mirroring the kernel's count_parameter_markers. 214 sea unit tests pass; all three fixes verified live against a warehouse. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…opped) `ExecuteStatementOptions.queryTags` is a public option but the SEA executeStatement never forwarded it, so tags were silently dropped versus the Thrift backend. Serialise them JS-side via `serializeQueryTags` into the conf overlay's `query_tags` key (the same wire shape Thrift produces) — not via the napi `queryTags` field, which is a `HashMap<String,String>` that can't represent a null-valued tag, and the kernel rejects setting both the field and a `query_tags` conf key. Null-valued tags round-trip as key-only segments. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
msrathore-db
changed the title
Contributor
Author
|
Scope note: this PR now also folds in two sets of fixes that were previously downstream PRs but are really fixes for code this PR (and #404) introduce, so a reviewer here would otherwise flag them:
The downstream stack (#406 → #407 → #408) was rebased onto this updated branch. |
This was referenced May 31, 2026
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Driver-side SEA↔Thrift parity, validated by the node comparator (thrift-vs-SEA) on a live warehouse. Stacked on
msrathore/sea-execution-metadata(#404, the metadata milestone). The interval + param behaviour needs the kernel napi knobs from databricks-sql-kernel#96 (intervalsAsString) and #84 (positionalParams); the integration branchmsrathore/krn-all-integrationhas both.1. Positional query parameters (wired through napi)
Removes the M0 param deferral.
SeaSessionBackend.executeStatementforwards positional?bindings to the kernel viaExecuteOptions.positionalParams(#84'sTypedValueInput {sqlType, value?}). NewSeaPositionalParams.tsreduces eachDBSQLParameter | valueto that shape — reusingDBSQLParameter's type-inference + stringification — adapting DECIMAL →DECIMAL(p,s)(the codec requires the parenthesised form) and NULL → value-less VOID. Also wiresqueryTimeout → queryTimeoutSecs. Named params stay rejected (napi surface is positional-only). AllPREPARED_STATEMENT_TYPEScases bind correctly live (INT/BIGINT/DECIMAL/STRING/BOOLEAN/DATE/TIMESTAMP/NULL + intervalCAST).2. Intervals → match Thrift
Default
intervalsAsString: trueon the SEA connection (SeaAuth) so INTERVAL columns surface as strings like the Thrift driver, and map a physical-Utf8column carrying INTERVAL type-name metadata →STRINGinSeaArrowIpc. Result: all 32SELECT *columns match Thrift in type and value.3. getInfo → synthesized (JDBC-style)
SeaServerInfo.tssynthesizes the threeTGetInfoTypes the Thrift server answers —CLI_SERVER_NAME/CLI_DBMS_NAME="Spark SQL",CLI_DBMS_VER="3.1.1"— byte-identical to Thrift, and rejects the rest as the server does.4. SQL-error class → match Thrift
Kernel
SqlError(PERMISSION_DENIED, SCHEMA_ALREADY_EXISTS, bad SQL) →OperationStateError(ERROR)instead of baseHiveDriverError, matching the Thrift operation-status error class.Tests
201 sea unit tests pass (new
positionalParams.test.ts,serverInfo.test.ts, interval-mapping inSeaIntervalParity,SqlError→OperationStateErrorinerror-mapping, getInfo +intervalsAsString+ param-forwarding inexecution/auth-*).Stack
sea-integration-foundation→ #404 (metadata) → this.This pull request and its description were written by Isaac.
Downstream fixes / reviewer note
getInfo, SQL-error class parity, input validation, and queryTags forwarding.queryTagsis serialized intostatementConf.query_tagsso null-valued tags match the Thrift wire shape.rowLimit, generalstatementConf, TIMESTAMP_NTZ/LTZ params).flatbuffers, one-pass IPC duration rewrite, cancel/close local-state rollback on native RPC failure, and closed fetch →OperationStateError(Closed).useCloudFetchis rejected in the later cascaded branches rather than silently ignored.SeaOperationBackendin this PR synthesizes a constantFINISHEDstatus, uses a client-sideuuidv4()as the statement id, andexecuteStatementblocks on the kernelexecute(). These are replaced by the real async model in feat(sea): execute statements asynchronously (submit + poll) for Thrift runAsync parity #406 (stacked on top): real serverstatus()(Pending/Running/Succeeded/Failed), the server-issuedstatementId, andsubmitStatement+ poll. A reviewer of this PR should not block on the synthesized status / fake id — they are intentional intermediate behaviour resolved in feat(sea): execute statements asynchronously (submit + poll) for Thrift runAsync parity #406.